fix(storage): treat a node as full only when disk and store are both out of room - #210
Conversation
…out of room A node decided it was full from `fs2::available_space()` alone and never asked LMDB whether the write would actually fit. Deleting a record returns its pages to LMDB's free list and never to the filesystem, so a node that has pruned heavily sits on reusable capacity while `statvfs` still reports the volume as full. It refused every write anyway, including writes that would land in a freed page without growing `data.mdb` by a byte. On the production fleet one host crossed the 500 MiB reserve and logged 423,202 write rejections in six hours across all 13 of its nodes, having pruned 4,609 records in the preceding day. The store had room; the guard could not see it. The predicate now has both halves. Below the reserve the map is pinned to the file's high-water mark, so a put succeeds exactly when LMDB can serve it from the free list and returns `MDB_MAP_FULL` the moment it would extend the file. The allocator is the authority, not an estimate: no page count can account for the copy-on-write of the B-tree path, the contiguous run a multi-megabyte value needs, or pages still pinned by an open read transaction. `check_capacity` remains a cheap pre-check and stays biased towards admitting. It estimates reusable bytes from `env.stat()` and refuses only when there is not one chunk's worth, preserving the saving of rejecting a full node before payment verification without blinding one that still has room. It deliberately avoids heed's `non_free_pages_size()`, which walks the unnamed database calling `String::from_utf8(key).unwrap()` and so panics on 32-byte binary keys. Two hazards the pinned mode introduces are handled explicitly: - A delete is itself a write. On a store with no free page it cannot copy-on-write inside a map pinned to the file size, so the node could never prune its way out. `delete` raises the ceiling by a budgeted allowance, retries, and restores it inside one exclusive-lock scope, with RAII guards so neither the ceiling nor the allowance can leak on an unwind. The allowance is charged only when the delete commits, because that is the only outcome whose copy-on-write can have extended the file permanently. - A `spawn_blocking` body outlives a cancelled awaiter, so an async lock cannot order two resizes. The mode's intent is published before the work, and both resize closures re-read it under the exclusive lock and decline if it has since been reversed. `try_resize` also measures the disk inside the closure, so a late one sizes from the disk as it is rather than as it was. Reviewed adversarially over six rounds; the findings on cross-size verdict caching, permanent map slack, transition races, torn reads and leak paths are all addressed.
b73a09a to
154863d
Compare
A store pinned to its file size cannot always copy-on-write a delete, so the delete path offers a temporary ceiling raise. That raise was budgeted one grant per low-disk episode, on the assumption that the first assisted delete frees pages the next one reuses. That assumption is wrong. LMDB will not hand back pages a still-recent transaction freed, so consecutive deletes on a full store can each need a little room. Charging per grant therefore stopped a node pruning after its first assisted delete, which is the opposite of what the allowance exists for. It passed locally and failed in CI because the two differ in page size: 16 KiB pages left enough slack in the first grant to cover later deletes, 4 KiB pages did not. What needs bounding is permanent file growth, since LMDB never returns file space, not the number of times slack was offered. The allowance is now charged the bytes `data.mdb` actually gained, measured across the delete. A delete that finds room inside the file costs nothing and pruning continues indefinitely, while repeated fill-then-delete cycles are still stopped from walking the file into the disk reserve. The test that pruned a pinned store now also asserts the accounting rule directly, so a regression to per-grant charging fails on any page size rather than only on hosts with small pages.
…e predicate The replication verification cycle gates its close-group probe on `LmdbStorage::capacity_verdict`, while `execute_single_fetch` gates the dial on `LmdbStorage::check_capacity`. The two restate one comparison rather than sharing it, so they can drift apart, and a verdict stricter than the pre-check is the harmful direction: a node the pre-check would let write stops discovering holders for keys it could have stored, which is under-replication rather than a saved probe. That is not hypothetical. WithAutonomi#210 makes the pre-check two-part, below the reserve and out of reusable pages inside the store, because LMDB returns a deleted record's pages to its own free list and never to the filesystem. A heavily pruned node fails only the first half, so with WithAutonomi#210 in and the verdict left as it stands, such a node stands its keys down for five minutes at a time instead of looking for chunks it can store. Add a unit test built on that state rather than on a bare full disk, where the two predicates still agree and a test would pass straight through the divergence. It writes and deletes two chunks so the store carries more than one chunk of reusable space, establishes the below-reserve precondition without either function under test, and asserts the two refuse together. Applying WithAutonomi#210's predicate to the pre-check fails this test and nothing else in the 934-test suite. Record the coupling in ADR-0011, as a trade-off, a validation entry and a review trigger, so the constraint outlives the pull request that found it.
dirvine
left a comment
dirvine
left a comment
There was a problem hiding this comment.
APPROVE — reviewed exact head 0a8540418ce518c033fc0211f5f4cb7188019230.
No material blockers found. Map sizing accounts for live data and reusable pages; runtime resize and all database operations are coordinated under env_lock; cancellation paths re-check growth mode; deletion retains bounded COW slack while full; restart restores the intended high-water behaviour. CI is green. Local verification: formatting and all 23 focused LMDB tests passed.
Integration note with #207: preserve #210's two-part capacity predicate when reconciling capacity_verdict; the cross-PR tripwire test should remain green.
Testnet evidence — PASS, with one observability findingTested at scale on a 195-service testnet (V2-1055, DEV-01 run 538), combined with #207 on branch Setup: 39 OVH VMs × 5 services; 10 VMs (50 nodes, 25.6%) artificially storage-full at The two-part predicate is live and correctDirectly observable on the real decision path. Every rejection on the full cohort carries it verbatim: The Clean no-growth exitOn the freed VMs after the volume went back above the reserve, over the following hour:
All five literals were confirmed present in the pinned source first, so these are genuine behavioural absences rather than renamed strings. Store files grew again immediately — 9.4 – 12.8 GB within the hour, against 67 – 95 MiB on the untouched still-full control — with no restart ( No regression above the reserveThis PR claims byte-for-byte unchanged behaviour on healthy nodes; the 145 healthy services support that. Fetch lane 16.41 / 15.79 / 15.41 GB/h — within 1.5% of the V2-987 reference arm's 16.65 GB/h for the same hour. Fleet fetch requests-to-responses 1:1. Downloads 2,284 / 2,284 = 100%. 0 restarts, 0 failed units, 0 panics across all 39 VMs / 195 services. No new WARN/ERROR patterns from the resize/pin/delete-allowance machinery. Finding: the
|
Carries WithAutonomi#210's two-part fullness predicate into the capacity verdict, as this PR's own notes require of whichever change lands second: capacity_verdict() now consults reusable_bytes() when the disk half reads Full, so the verification gate and the dial pre-check refuse on the same condition and a pruned node below its reserve keeps discovering holders for keys it can still store. check_disk_space() is dropped (WithAutonomi#210 removed its only callers; this branch added none). The tripwire test capacity_verdict_refuses_exactly_when_check_capacity_does passes on the divergent (pruned) state. cargo test --lib: 941/941. e2e write_blocked_node_neither_probes_nor_dials passes. Tree is identical to the V2-1055 testnet build (jacderida/ant-node 239aa94). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
3 participants
Linear issue
https://linear.app/autonominetwork/issue/V2-1034/capacity-check-ignores-reusable-lmdb-pages-so-pruned-nodes-refuse
Risk tier
This is a bug fix restoring the intended capacity check, not a new design: the two-part predicate ("full" = no disk and no reusable page) was always what the guard was supposed to express, and the
MapFullhalf was already implemented and simply unreachable. No wire message, stored format, payment path or upgrade mechanism changes, and behaviour above the disk reserve is byte-for-byte unchanged.Reviewer note: the only behavioural delta is on a node already under its reserve, which today refuses every write. Flag it if you read that as T2.
Compatibility
check_capacitykeeps its signature and itsInsufficient disk space …error text; only the condition under which it fires changes.Semver impact
Test evidence
cargo test --lib: 936 passed, 0 failed.cfd(fmt + clippy + doc) clean.Seven tests added or rewritten, each pinned to a claim rather than to the implementation:
below_reserve_put_reuses_freed_pagesdata.mdbdoes not grow doing it.below_reserve_refused_put_does_not_grow_the_filelarge_refusal_does_not_block_a_smaller_putfull_store_below_reserve_can_still_deletecheck_capacity_tracks_reusable_space_not_just_diskstatvfsunchanged throughout.leaving_no_growth_restores_head_roomabove_reserve_behaviour_is_unchangedtest_put_rejected_on_insufficient_capacity_before_verification(handler) still proves a genuinely full node short-circuits ahead of payment verification, which is the saving the earlier pre-check work introduced.Adversarially reviewed over six rounds. Findings addressed rather than argued: a store-wide
MapFullverdict that let one large chunk lock out small writes; a permanent map slack that was really ordinary put capacity and multiplied by nodes per volume; transition races between the mode flag and the map size; a torn read in the reusable estimate that could under-report; and leak paths for both the raised ceiling and the delete allowance under error, panic and cancellation.Not yet done: no testnet run. Worth exercising on a deliberately filled host before it rides a train, because the behaviour only differs once a volume is under its reserve.
New dependency
none
ADR
n/a — bug fix, not an architectural decision.
The reasoning that would have gone in one is in the commit message and in the code comments at the decision points: why the allocator answers "does this write fit" instead of a page-count estimate, why
non_free_pages_size()must not be called, why a delete needs a budgeted allowance, and why an async lock cannot order two resizes.Mitigation / rollback
Revert the commit. The change is confined to
src/storage/lmdb.rsand one handler test, adds no state that outlives the process, and writes nothing new to disk. Behaviour above the disk reserve is unchanged, so the blast radius is limited to nodes already under their reserve, which today refuse every write regardless.